Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#4759] feat(auth-ranger): Use Spark verify Ranger authorization Hive #4948

Merged
merged 1 commit into from
Sep 27, 2024
Merged

[#4759] feat(auth-ranger): Use Spark verify Ranger authorization Hive #4948

merged 1 commit into from
Sep 27, 2024

Conversation

xunliu
Copy link
Member

@xunliu xunliu commented Sep 16, 2024 •
edited
Loading

What changes were proposed in this pull request?

  1. Shadow integration-test-common module.
  2. Add Ranger authorization Hive IT, use Spark to verify Ranger authorization Hive.

Why are the changes needed?

Fix: #4759

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

CI Passed.

@xunliu xunliu requested review from mchades and xloya September 16, 2024 03:18
@xunliu xunliu self-assigned this Sep 16, 2024
@xunliu xunliu force-pushed the issue-4759 branch 11 times, most recently from bb0092c to d399416 Compare September 16, 2024 11:30
@xunliu xunliu force-pushed the issue-4759 branch 11 times, most recently from 72789d7 to dba750d Compare September 18, 2024 15:20
@xunliu xunliu changed the title [#4759] feat(CI): Use Spark verify Ranger authorization Hive [#4759] feat(auth-ranger): Use Spark verify Ranger authorization Hive Sep 18, 2024
@xunliu xunliu force-pushed the issue-4759 branch 2 times, most recently from 71e1859 to b7675dc Compare September 23, 2024 11:45
xunliu
xunliu commented Sep 23, 2024
View reviewed changes
Copy link
Member Author

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FANNG1 @yuqi1129
Please help me review again, thanks.

@FANNG1
Copy link
Contributor

FANNG1 commented Sep 24, 2024 •
edited
Loading

The current shade implement seems to lead to some unexpected behavior, I'm afraid it makes the test class problems more complicated. One possible solution may shaddle all depenences in integration-test-common.

@FANNG1
Copy link
Contributor

FANNG1 commented Sep 24, 2024

BTW, I prefer to drop the support for MiniGravitino to avoid to struggle for the class conflict related problems. @mchades @yuqi1129 @xunliu WDYT?

@mchades
Copy link
Contributor

mchades commented Sep 24, 2024 •
edited by FANNG1
Loading

Do you mean to remove the embedded tests?

@FANNG1
Copy link
Contributor

FANNG1 commented Sep 24, 2024

Do you mean to remove the embedded tests?

Yes, MiniGravitino provides the facilities to debug easily, but leading complicated class conflict problems, it takes much more time to debug to me.

@xunliu
Copy link
Member Author

xunliu commented Sep 24, 2024

Do you mean to remove the embedded tests?

Yes, MiniGravitino provides the facilities to debug easily, but leading complicated class conflict problems, it takes much more time to debug to me.

I think MiniGravitino is still a necessity.

  1. Other systems will also require the use of MiniGravitino for integration testing. Like MiniHiveMetastore.
  2. Without miniGravitino, community developers would have to use remote debugging mode, which is too difficult.

I think the best way to do this is that we should create a separate MiniGravitino module.

@xunliu
Copy link
Member Author

xunliu commented Sep 24, 2024

@mchades

I am confused about why it worked before without needing an explicit call?

Because the module after the shadow, there is no longer this file structure META-INF/services/java.sql.Driver , so you can not find the h2database through the SPI, you must show the call Class.forName("org.h2.Driver"); function

image

@yuqi1129
Copy link
Contributor

@mchades

I am confused about why it worked before without needing an explicit call?

Because the module after the shadow, there is no longer this file structure META-INF/services/java.sql.Driver , so you can not find the h2database through the SPI, you must show the call Class.forName("org.h2.Driver"); function

image

@xunliu
The file exists, but the content in it is unexpected, please see #4948 (comment)

@xunliu
Copy link
Member Author

xunliu commented Sep 24, 2024

@xunliu need answer.

Because Jetty server default load https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-deploy/src/test/resources/etc/webdefault.xml#L37, But we rename org.eclipse.jetty to org.apache.gravitino.it.shaded.org.eclipse.jetty, So we load server-common/src/test/resources/web.xml in the test mode.

Don't worry, the Jetty server loads web.xml in the web module in the produce mode, There's no such thing.

@xunliu xunliu marked this pull request as draft September 25, 2024 00:30
@xunliu xunliu marked this pull request as ready for review September 26, 2024 08:19
@xunliu xunliu mentioned this pull request Sep 26, 2024
Open
@xunliu
Copy link
Member Author

xunliu commented Sep 26, 2024

@FANNG1 @mchades @yuqi1129 I refactored this PR, Please help me review it again, thanks.

yuqi1129
yuqi1129 reviewed Sep 26, 2024
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except a minor one.

...er/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerITEnv.java
static void startHiveRangerContainer() {
containerSuite.startHiveRangerContainer(
new HashMap<>(
ImmutableMap.of(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a nested Map?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, I already fixed it in the next PR.

@xunliu xunliu merged commit 46a6a20 into apache:main Sep 27, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchades mchades mchades left review comments

@yuqi1129 yuqi1129 yuqi1129 left review comments

@FANNG1 FANNG1 FANNG1 approved these changes

@xloya xloya Awaiting requested review from xloya

Assignees

@xunliu xunliu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Use Spark verify Ranger authorization Hive
4 participants
@xunliu @FANNG1 @mchades @yuqi1129